ARTEMIS-5836 - adding and enabling tests #154
Conversation
lavocatt
left a comment
lavocatt
left a comment
There was a problem hiding this comment.
Here's some comment, I used claude to help me understand the limitations of the current testing methods, some of the feedback on the naming of the functions are claude's, as well as the idea of using spies.
| }) | ||
|
|
||
| test("initialize should handle null search results", async () => { | ||
| const testService = new (artemisService.constructor as any)() |
There was a problem hiding this comment.
The instanciation of the object is weird to me, i don't really understand why we're not doing
const testService = new ArtemisService, the only thing that's missing to do that is to export the ArtemisService class so that it can be imported in the test case.
|
|
||
| const mockConfig = { jmx: { domain: 'org.apache.activemq.artemis' } } | ||
| jest.spyOn(configManager, 'getArtemisconfig').mockResolvedValue(mockConfig) | ||
| jest.spyOn(jolokiaService, 'search').mockResolvedValue(null as any) |
There was a problem hiding this comment.
I'm not found of null as any, it hides the fact that the correct type for the answer should be a string[]. Surely passing an empty array would be better.
| expect(brokerObjectName).toBe('') | ||
| }) | ||
|
|
||
| test("initialize should handle null search results", async () => { |
There was a problem hiding this comment.
Probably a better name for the test would be initialize should return empty string when no brokers found
| expect(brokerObjectName).toBe('') | ||
| }) | ||
|
|
||
| test("initialize should handle search errors gracefully", async () => { |
There was a problem hiding this comment.
A better name would be initialize should catch errors and return empty string but as we discussed, it would probably be preferable to fail the initialization in such case.
| expect(brokerObjectName).toBe('org.apache.activemq.artemis:broker=broker1') | ||
| }) | ||
|
|
||
| test("initialize should handle custom JMX domain from config", async () => { |
There was a problem hiding this comment.
Would this be a better name initialize should use custom JMX domain from config in search string?
| jest.spyOn(jolokiaService, 'search').mockResolvedValue([ | ||
| 'org.apache.activemq.artemis:broker=broker1', | ||
| 'org.apache.activemq.artemis:broker=broker2' | ||
| ]) |
There was a problem hiding this comment.
probably use more than two, to make sure it's not just a flip of a coin.
| jest.spyOn(configManager, 'getArtemisconfig').mockResolvedValue(mockConfig) | ||
| jest.spyOn(jolokiaService, 'search').mockResolvedValue(['org.apache.activemq.artemis:broker=test']) |
There was a problem hiding this comment.
You can make use of spyes to make sure the jest is actually called by every initialization methods.
| test("getBrokerObjectName should return empty string before initialization", async () => { | ||
| const testService = new (artemisService.constructor as any)() | ||
|
|
||
| // Don't call initialize | ||
| const brokerObjectName = await testService.getBrokerObjectName() | ||
|
|
||
| expect(brokerObjectName).toBe('') | ||
| }) |
There was a problem hiding this comment.
If the service returns an empty string before initialization, then I'd advocate even more for throwing an exception when receiving garbage data during the initialization phase.
Another question is, what happens if I attempt to read some data with an uninitialized artemisService, do I get an error?
2 participants
initially some simple tests to test framework and GitHub action